Skip to content

feat: add auth preprocessor and update auth handler.#227

Open
kalenkevich wants to merge 4 commits intomainfrom
feat/oauth_part3
Open

feat: add auth preprocessor and update auth handler.#227
kalenkevich wants to merge 4 commits intomainfrom
feat/oauth_part3

Conversation

@kalenkevich
Copy link
Copy Markdown
Collaborator

@kalenkevich kalenkevich commented Mar 31, 2026

Overview

This PR improves the test coverage and fixes inconsistencies in the authentication handling logic for the ADK-JS project. It addresses a bug in AuthHandler where exchange results were stored incorrectly, enhances existing tests to verify actual generated values (like URLs and states), and introduces a new unit test suite for the AuthPreprocessor.

Key Changes

Bug Fixes

AuthHandler

  • Removed a type inconsistency in parseAndStoreAuthResponse where the full ExchangeResult object was unintentionally stored in session state. Now, it correctly stores only the nested AuthCredential (exchangedCredential.credential), aligning with how getAuthResponse retrieves it.

Testing Enhancements

AuthHandler Tests

  • New Coverage: Added comprehensive unit tests for parseAndStoreAuthResponse to cover:
    • Non-OAuth2 storage mechanics.
    • Early-return conditions for unsupported schemes.
    • Successful OAuth2 exchanges.
  • Improved Assertions:
    • generateAuthUri tests now assert the actual generated URL string and query parameters (such as client_id, redirect_uri, scope, and dynamic state) rather than returning stubs.
    • generateAuthRequest tests assert usage of the generated credentials.

[NEW] AuthPreprocessor Tests

  • Created unit tests covering AuthPreprocessor.runAsync generator logic:
    • Verification of early-return scenarios:
      • Agent is not an LlmAgent.
      • Active session has no events or user content.
      • Message history contains no adk_request_credential tool-resume responses.
    • Successful tool resumption scenario: verifies the preprocessor intercepts a user's credential input, updates state storage, parses resume target tool calls, and delegates continuity via handleFunctionCallsAsync.
  • Standardized Vitest mocking conventions and TypeScript types for handleFunctionCallsAsync.

@kalenkevich kalenkevich self-assigned this Mar 31, 2026
Base automatically changed from feat/oauth_part2 to main April 1, 2026 21:25
@kalenkevich kalenkevich force-pushed the feat/oauth_part3 branch 2 times, most recently from c2292e3 to 917ee86 Compare April 1, 2026 21:29
ScottMansfield
ScottMansfield reviewed Apr 8, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@ScottMansfield ScottMansfield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm only partway through review but I thought it's best to leave what I have for now

core/src/auth/oauth2/oauth2_utils.ts
access_token?: string;
refresh_token?: string;
expires_in?: number;
id_token?: string;
Copy link
Copy Markdown
Member

@ScottMansfield ScottMansfield Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: ordering. Put this one line up

core/src/auth/auth_handler.ts
Comment on lines +31 to +38
if (this.authConfig.exchangedAuthCredential) {
state.set(credentialKey, this.authConfig.exchangedAuthCredential);
}

const authSchemeType = this.authConfig.authScheme.type;
if (!['oauth2', 'openIdConnect'].includes(authSchemeType)) {
return;
}
Copy link
Copy Markdown
Member

@ScottMansfield ScottMansfield Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to store the auth in the state even if it's not a supported auth scheme?

core/src/auth/auth_handler.ts

const authSchemeType = this.authConfig.authScheme.type;
if (!['oauth2', 'openIdConnect'].includes(authSchemeType)) {
return;
Copy link
Copy Markdown
Member

@ScottMansfield ScottMansfield Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this not an error if the scheme is invalid?

core/src/auth/auth_handler.ts
authCredential: this.authConfig.exchangedAuthCredential,
authScheme: this.authConfig.authScheme,
});
state.set(credentialKey, exchangedCredential.credential);
Copy link
Copy Markdown
Member

@ScottMansfield ScottMansfield Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is set twice and the check for the exchangedAuthCredential is done twice here. I think understanding this depends on the above comments I made.

core/src/auth/auth_handler.ts
Comment on lines +127 to +131
if ('authorizationUrl' in flow && flow.authorizationUrl) {
authorizationEndpoint = flow.authorizationUrl;
} else if ('tokenUrl' in flow && flow.tokenUrl) {
authorizationEndpoint = flow.tokenUrl;
}
Copy link
Copy Markdown
Member

@ScottMansfield ScottMansfield Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the authorizationCode flow, there's both authorizationUrl and tokenUrl defined, does authorizationUrl override tokenUrl?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ScottMansfield ScottMansfield ScottMansfield left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@kalenkevich kalenkevich

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@kalenkevich @ScottMansfield @Varun-S10